-
Notifications
You must be signed in to change notification settings - Fork 36
Replace Metadata.flags
with Metadata.trans
#1060
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Benchmark Report for Commit a011dd6Computer Information
Benchmark Results
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## breaking #1060 +/- ##
============================================
- Coverage 82.38% 82.36% -0.03%
============================================
Files 42 42
Lines 3820 3787 -33
============================================
- Hits 3147 3119 -28
+ Misses 673 668 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DynamicPPL.jl documentation for PR #1060 is available at: |
CI benchmarks. Target branch:
This branch:
Roughly in line with what I saw locally. Seems worth it to me, especially if you look at the Loop univariate 1k and 10k models. |
I suggest we take this chance to rename |
Good idea, done. |
src/simple_varinfo.jl
Outdated
islinked(vi::SimpleVarInfo) = istrans(vi) | ||
islinked(vi::SimpleVarInfo) = is_transformed(vi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like this line is just the same function but duplicated. so it feels like to me we could just pick one and roll with it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to merge after this! Very pleased with the improved performance
@mhauru I'll leave you to click the green button :) |
Now that the
"del"
flag is gone (#1058), the only flag that is ever used is"trans"
. Hence, no need to bother with having theDict{String, BitVector}
forMetadata.flags
, and can instead have a singleBitVector
forMetadata.trans
. EDIT: Renamed toMetadata.is_transformed
.You may wonder, given that
Metadata
is presumably on its way out, why bother? Two reasons:VectorVarInfo
, and there were some horrendous performance regressions there compared to usingMetadata
. Hence, we might not be about to switch over theVarNamedVector
imminently.Metadata.flags
field might actually be a significant cost compared to aBitVector
.My local benchmarking suggests that indeed, this makes a difference:
Before
After
Curious to see whether GHA benchmarks come out looking similar.